Skip to content

fix(hot-reload,devtools): recover from hook-order edits; default DevtoolsMenu items to null#186

Merged
codemonkeychris merged 2 commits into
mainfrom
fix/hot-reload-recovery-and-devtools-default
May 7, 2026
Merged

fix(hot-reload,devtools): recover from hook-order edits; default DevtoolsMenu items to null#186
codemonkeychris merged 2 commits into
mainfrom
fix/hot-reload-recovery-and-devtools-default

Conversation

@codemonkeychris
Copy link
Copy Markdown
Collaborator

Summary

Two related issues surfaced while putting together a demo app:

  1. Hot Reload now survives hook-order / hook-type edits. Adding, removing, or reordering hooks (or changing a hook's generic type) used to leave the app stuck on the error fallback after the very next save — the surviving RenderContext had the previous hook list pinned by index. The dev loop died until the user restarted. New behavior: when a hook mismatch surfaces during a render triggered by .NET Hot Reload, the host runs effect cleanups, drops the hook list, and re-renders from scratch. State is lost (cannot reliably re-key surviving values onto a changed shape), but the app keeps running.

  2. DevtoolsMenu() works with no arguments. Calling it with no args used to require a no-op () => Array.Empty<MenuFlyoutItemBase>() to get the built-in toggles. The implementation already null-coalesced internally; only the parameter signature blocked it.

Why one PR

Both surfaced in the same demo session. Together they're "things that should just work when you're prototyping." Splitting felt like ceremony for two tightly related quality-of-life fixes. Happy to split if reviewers prefer.

Hot Reload recovery — design

  • New HookOrderException : InvalidOperationException so the host can sniff the specific subtype without changing existing catch (InvalidOperationException) semantics for callers.
  • 10 hook-order throw sites in RenderContext.cs converted from InvalidOperationException to HookOrderException. Cross-thread setter and UseNavigation lookup throws are intentionally left as plain InvalidOperationException — neither is a hot-reload-recoverable failure.
  • RenderContext.ResetForHotReload() (internal): runs cleanups, clears hook list, resets index.
  • HotReloadService.UpdatePending: signals "the next render is a hot-reload-induced render." Set by UpdateApplication, consumed (capture-and-clear) by ReactorHostControl.Render at the top.
  • ReactorHostControl.Render() picks up the flag and adds catch (HookOrderException) when (hotReloadRender) recovery filters on each render branch.

Infinite-loop guard

The capture-and-clear pattern guarantees at-most-once recovery per UpdateApplication call:

UpdateApplication fires:    UpdatePending = true
Render runs:                hotReloadRender = true,
                            UpdatePending = false  (consumed)
  hooks throw:              recover + RequestRender, return
Recovery render runs:       hotReloadRender = false  (already consumed)
  hooks throw again:        falls through `when (hotReloadRender)`
                            filter → ShowErrorFallback

A developer who saved genuinely broken hook code sees the error fallback once, not an infinite reset loop. Each subsequent save grants exactly one more retry.

Test plan

  • dotnet test tests/Reactor.Tests — 6821 passed, 0 failed (2 new for the recovery path)
  • dotnet test tests/Reactor.SelfTests — pending (running)
  • 5 existing Assert.Throws<InvalidOperationException> hook-order tests tightened to Assert.Throws<HookOrderException>. The looser assertion was incidental — these tests have always been about a specific contract that now has a specific type.
  • 2 new tests in HookStateRefactorTests.cs: ResetForHotReload_RunsCleanups_And_ClearsHooks_So_Next_Render_Sees_Fresh_Sequence and ResetForHotReload_Allows_Different_Hook_Type_At_Same_Index.
  • Recommended manual: run dotnet watch against a Reactor sample, edit a component to add/remove/reorder a hook, save, confirm the app reloads instead of hard-failing.

🤖 Generated with Claude Code

…oolsMenu items to null

Two related fixes surfaced while putting together a demo app:

1. Hot Reload survives hook-order / hook-type edits.

   Editing a component to add, remove, or reorder hooks (or change a
   hook's generic type) used to leave the app stuck on the error
   fallback after the very next save — the surviving RenderContext had
   the *previous* hook list pinned by index, and the new Render() body
   couldn't line up against it. The dev loop died until the user
   restarted the app.

   New behavior: when a hook-order/type mismatch surfaces during a
   render that was triggered by .NET Hot Reload, the host runs effect
   cleanups, drops the hook list, and re-renders from scratch. Hook
   state is lost (we cannot reliably re-key arbitrary surviving values
   onto a shape that may have changed) but the app keeps running.

   - New HookOrderException : InvalidOperationException so the host
     can sniff the specific subtype without changing existing
     `catch (InvalidOperationException)` semantics for callers.
   - All 10 hook-order/type-mismatch throw sites in RenderContext.cs
     converted from InvalidOperationException to HookOrderException.
     Cross-thread setter and UseNavigation lookup throws are
     intentionally left as plain InvalidOperationException — neither
     is a hot-reload-recoverable failure.
   - RenderContext.ResetForHotReload(): runs cleanups, clears hook
     list, resets index. New API, internal-only.
   - HotReloadService.UpdatePending: signals "the next render is a
     hot-reload-induced render". Set by UpdateApplication, consumed
     (capture-and-clear) by ReactorHostControl.Render at the top of
     the render attempt.
   - ReactorHostControl.Render(): catch (HookOrderException) when
     (hotReloadRender) recovers; any subsequent failure on the
     recovery render falls through to the existing ShowErrorFallback
     path. The capture-and-clear pattern guarantees at-most-once
     recovery per UpdateApplication, so genuinely broken hook code
     surfaces the error fallback once instead of an infinite reset
     loop.

   Tests:
   - 5 existing Assert.Throws<InvalidOperationException> hook-order
     tests tightened to Assert.Throws<HookOrderException>. The looser
     assertion was incidental — these tests have always been about a
     specific contract that now has a specific type.
   - 2 new tests in HookStateRefactorTests.cs:
     ResetForHotReload_RunsCleanups_And_ClearsHooks_So_Next_Render_Sees_Fresh_Sequence
     and ResetForHotReload_Allows_Different_Hook_Type_At_Same_Index.

2. DevtoolsMenu items factory defaults to null.

   Calling DevtoolsMenu() with no arguments to get the built-in
   toggles (Highlight reconcile / Show layout cost) used to require
   passing a no-op `() => Array.Empty<MenuFlyoutItemBase>()`. The
   implementation already null-coalesces inside (`items?.Invoke()
   ?.ToArray() ?? Array.Empty<...>()`) — the parameter was just
   non-nullable. Changed to `Func<...>? items = null` so
   `DevtoolsMenu()` with no args is valid.

Test results:
- dotnet test tests/Reactor.Tests: 6821 passed, 0 failed (2 new).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codemonkeychris codemonkeychris requested a review from Copilot May 7, 2026 14:07
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR improves the developer experience during .NET Hot Reload by recovering from hook-order/type edits (resetting hook state + rerendering) and makes DevtoolsMenu() callable without providing a no-op items factory.

Changes:

  • Introduces HookOrderException and updates hook-order throw sites + tests to use it.
  • Adds hot-reload recovery flow: RenderContext.ResetForHotReload() + host-side catch-and-rerender gated by a “hot reload render” flag.
  • Updates DevtoolsMenu factory signature to allow items to be omitted (null default).
Show a summary per file
File Description
tests/Reactor.Tests/PersistedStateTests.cs Updates hook-order assertion to expect HookOrderException.
tests/Reactor.Tests/NavigationLifecycleTests.cs Updates hook-order assertion to expect HookOrderException.
tests/Reactor.Tests/HookStateRefactorTests.cs Updates exception expectations and adds new tests for ResetForHotReload() recovery behavior.
tests/Reactor.Tests/ContextSystemUnitTests.cs Updates hook-order assertion to expect HookOrderException.
src/Reactor/Hosting/ReactorHostControl.cs Captures/clears hot-reload flag and adds recovery catch blocks that reset context + rerender.
src/Reactor/Hosting/HotReloadService.cs Adds UpdatePending flag and sets it during UpdateApplication.
src/Reactor/Hosting/Devtools/DevtoolsMenuFactory.cs Makes items optional (null default) for DevtoolsMenu.
src/Reactor/Core/RenderContext.cs Throws HookOrderException at hook mismatch sites and adds ResetForHotReload().
src/Reactor/Core/HookOrderException.cs Adds new exception type used to identify hook-order/type mismatches.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 9/9 changed files
  • Comments generated: 5

Comment thread src/Reactor/Hosting/ReactorHostControl.cs Outdated
Comment thread src/Reactor/Hosting/HotReloadService.cs Outdated
Comment on lines 72 to 75
if (_hooks[currentIndex] is not ValueHookState<T> hook)
throw new InvalidOperationException(
throw new HookOrderException(
$"Hook at index {currentIndex} is {_hooks[currentIndex].GetType().Name}, expected ValueHookState<{typeof(T).Name}> (UseState). " +
"Hooks must be called in the same order every render.");
Comment thread src/Reactor/Hosting/ReactorHostControl.cs
Comment thread src/Reactor/Hosting/ReactorHostControl.cs
- HotReloadService.UpdatePending now backed by an int with
  Volatile.Write/Read on the producer side and
  Interlocked.Exchange via ConsumeUpdatePending() on the consumer
  side. UpdateApplication runs on a hot-reload runtime thread while
  Render runs on the UI dispatcher; the previous read-then-write
  pattern had a window where a concurrent UpdateApplication could
  raise the flag between Render's read and clear, losing the new
  pending update. Single CAS removes the window.

- HotReloadService XML doc rewritten to match the actual contract:
  the flag is consumed (atomically) at the start of each render
  attempt, not "after the render completes" as the previous comment
  said.

- ReactorHostControl.Render: extracted a RecoverFromHookOrder local
  function so the two HookOrderException catch branches
  (component-mode / function-mode) share a single log → reset →
  RequestRender sequence. Future tweaks (telemetry, throttling,
  additional reset steps) edit one place.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codemonkeychris
Copy link
Copy Markdown
Collaborator Author

Update pushed (0c72f51). Working through the five comments:

1 — TOCTOU on capture-and-clear (ReactorHostControl.cs:348-349): Fixed. HotReloadService.UpdatePending is now backed by an int with Volatile.Write on the producer side and Interlocked.Exchange via the new ConsumeUpdatePending() on the consumer side. Single CAS, no window. The reviewer is right that this matters: UpdateApplication runs on a hot-reload runtime thread, Render runs on the UI dispatcher, and the previous non-atomic pattern could lose a pending update if a second save fired in the read-then-write gap.

2 — XML doc on UpdatePending (HotReloadService.cs:21): Fixed. Doc rewritten to match the actual contract — the flag is consumed (atomically) at the start of each render attempt, not "after the render completes" as the previous comment incorrectly said.

3 — UseStateSetterByIndex classification (RenderContext.cs:72-75): Disagreeing. The lines pointed at (L72–75) are inside UseState (the public hook, defined at L62–114), not UseStateSetterByIndex (which is the DEBUG-only test helper at L46–53 and doesn't throw any hook-order exceptions — it just does an if (index < _hooks.Count && _hooks[index] is ValueHookState<T> hook) lookup and returns silently if the index/type doesn't match). The conversion at L73 is correct: it's the standard UseState hook-order check, which is exactly what HookOrderException is for. The cross-thread setter path I called out in the PR description is AssertUIThread at L37 — that one was correctly left as InvalidOperationException (you can verify with a git diff on RenderContext.cs:37). I think there was a line-number scroll mismatch in the review tool here.

4 & 5 — Dedup the two catch blocks: Fixed. Extracted a local function RecoverFromHookOrder(ex, ctx, mode) inside Render(). Both HookOrderException catch branches now call it, so the log → ResetForHotReloadRequestRender sequence lives in one place. Future changes (telemetry, throttling, additional reset steps) edit one location.

Test gates: 6821 unit tests pass.

@codemonkeychris codemonkeychris merged commit bb85642 into main May 7, 2026
6 checks passed
codemonkeychris added a commit that referenced this pull request May 7, 2026
#186 added hot-reload hook-order recovery to ReactorHostControl but
missed ReactorHost — the path used by `ReactorApp.Run<T>(...)`. Apps
launched via the app-builder (the demo template, samples, every
non-XAML-Islands consumer) hit a HookOrderException on the first hot
reload that adds, removes, or reorders a hook, fall through to the
generic catch, and land on the error fallback until the user
restarts the process.

Mirror the same recovery sequence in ReactorHost.Render():

- Capture HotReloadService.ConsumeUpdatePending() once at the top.
- Add a `catch (HookOrderException ex) when (hotReloadRender)` filter
  ahead of the generic catch on both component-mode and
  function-component-mode branches.
- Recovery runs effect cleanups, drops the hook list
  (RenderContext.ResetForHotReload), and RequestRenders — same
  contract as ReactorHostControl, so devs working on a hook-shape
  edit see one warning log and the app keeps rendering.
@codemonkeychris codemonkeychris deleted the fix/hot-reload-recovery-and-devtools-default branch May 28, 2026 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants